scan all versions in multi-version-jar #61
Conversation
|
@rPraml : thanks for contributing.
See https://github.com/eclipse-jdt/.github/blob/main/CONTRIBUTING.md |
There was a problem hiding this comment.
I've identified 3 places, where multi-release-jars are not properly handled according to the spec.
These changes should make ecj more javac comatible.
It would be great, if someone can pick up this work and write a test, as I have not much knowledge how to do this in the eclipse ecosystem
| if (!Files.exists(this.releasePath)) { | ||
| this.releasePath = null; | ||
| for (int version = Integer.parseInt(this.compliance); version >= 9; version--) { | ||
| this.releasePath = this.fs.getPath("/", "META-INF", "versions", String.valueOf(version)); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ |
There was a problem hiding this comment.
according to
https://docs.oracle.com/javase/10/docs/specs/jar/jar.html#multi-release-jar-files
all versions down to 9 have to be checked.
this.compliance should be always a major version accroding to the spec, so parseInt should work
There was a problem hiding this comment.
Probably it would be good to parse the compliance only once (same below) and cache it in a field?
| ZipEntry entry = zipFile.getEntry(releasePath); | ||
| if (entry != null) { | ||
| path = releasePath; | ||
| for (int version = Integer.parseInt(release); version >= 9; version--) { |
There was a problem hiding this comment.
This is practically the same code as above.
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| // move on to the default | ||
| for (int version = Integer.parseInt(this.compliance); version >= 9; version--) { |
There was a problem hiding this comment.
and here, we search for the module-info.class, which should be the fix for the bug
There was a problem hiding this comment.
Any reason why It's being done in ClasspathJar instead of org.eclipse.jdt.internal.core.builder.ClasspathMultiReleaseJar ?
There was a problem hiding this comment.
@jarthana Do you mean, why I have made this change or why this code is here at all?
IMHO it sounds logical, if this is handled in ClasspathMultiReleaseJar as it does not require to have duplicate code.
(But I'm really not the expert to do such refactoring in this project)
There was a problem hiding this comment.
I was just wondering about having this in the super class ClasspathJar but in case of the compiler.batch package, you have made it in the subclass ClasspathMultiReleaseJar. Haven't looked at the code in detail, but IIRC, we had similar code and hierarchy in both the compiler.batch and core.builder packages.
This PR refers to
https://bugs.eclipse.org/bugs/show_bug.cgi?id=577790